Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EXPERIMENT: builder APIs via bon #96

Draft
wants to merge 33 commits into
base: trunk
Choose a base branch
from

Conversation

ErichDonGubler
Copy link
Collaborator

@ErichDonGubler ErichDonGubler commented Sep 10, 2024

This PR is an experiment that uses bon's derive APIs

Steps to take to advance this experiment:

  • Get user feedback.
  • Draw the rest of the owl with the APIs that don't have builders yet.
    • Render pipelines
    • Compute pipelines
    • What else?
  • Maybe try bon's method builders (as separate methods from the existing ones)?

Warning

Heavily WIP, and, notably, not a priority for my work at Mozilla.

@Veetaha
Copy link

Veetaha commented Sep 10, 2024

Hi @ErichDonGubler, I'm the maintainer of bon. I just discovered this heroic effort through the github search 👀. Hopefully that was a fun experience 😸. I wonder if you have any feedback and most importantly discovered bugs / annoyances / suggestions to improve bon as a result of this?

pub struct RenderPipelineDescriptor<'a> {
/// Debug label of the pipeline. This will show up in graphics debuggers for easy identification.
#[builder(default, into)]
Copy link

@Veetaha Veetaha Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that Label is just a type alias for Option<&str>, I'd suggest to inline that type alias so that bon recognizes it as an Option under the hood (it just uses a heuristic text match by the type's path to detect an Option).

For example with the current

#[builder(default, into)] label: Label<'a>

the following setters will be generated:

fn label(self, value: impl Into<Option<&str>>) -> NextBuilderState
fn maybe_label(self, value: Option<impl Into<Option<&str>>) -> NextBuilderState

which adds unnecessary Option nesting. If you inline the type alias, you can omit any #[builder] configs as well:

label: Option<&'a str>

and this will produce the following setters:

fn label(self, value: &str) -> NextBuilderState
fn maybe_label(self, value: Option<&str>) -> NextBuilderState

which removes the Option nesting, and greatly simplifies the code

@ErichDonGubler ErichDonGubler force-pushed the easier-ctors/bon-builder branch from afd6085 to 2f9f51c Compare September 19, 2024 16:18
@Veetaha

This comment was marked as resolved.

@ErichDonGubler ErichDonGubler force-pushed the easier-ctors/bon-builder branch 3 times, most recently from e77244d to 577ee3a Compare September 20, 2024 18:15
@Veetaha
Copy link

Veetaha commented Sep 20, 2024

Thank you for the background. I'm not familiar with wgpu (my background is in the distributed systems in the cloud), but I'd be glad to help with any effort of using bon to improve the API surface of the wpgu:

  • Assist with code reviews, design recommendations and resolving any questions
  • Implement new features / fix existing bugs and annoyances in bon that may hinder this use case
  • I may also help with code changes to extend wgpu with the builder syntax APIs unless you'd like to cover all of that on your side.

So from the context you've given, I think that using derive API with structs is the ultimate way to go here. This allows you to keep the API backward compatible, and keeps it much more in line with the WebGPU spec. This will also make it easier for anyone else to maintain this API (no separate method-based builder syntax to maintain).

I've also heard opinions from people that "flat" method call API like this:

.create_texure()
.param(...)
.param(...)
.build()

doesn't look very nice when this method chain is extended with other similar builder API calls:

.create_texure()
.param(...)
.param(...)
.build()
.do_smth_else()
.other_method_param(...)
.other_method_param(...)
.call()
.other_method()
.param(...) 
// ...

Instead if the API is supposed to be chained a lot, then the "Descriptor" parameter structs syntax makes it easier to read with additional small nesting:

.create_texure(
    TextureDescriptor::builder()
        .param(...)
        .param(...)
        .build()
)
.do_smth_else(
    OtherDescriptior::builder()
        .other_method_param(...)
        .other_method_param(...)
        .build()
)
.other_method(
    OtherDescriptior2::builder()
        .other_method_param(...)
        .other_method_param(...)
        .build()
)

The style of "flat builder" API with method-based builders was inspired by AWS SDK API. In that case it makes sense since API calls in AWS SDK are not chained with each other, so such readability problem doesn't appear.

Also, the descriptor-struct-based API makes it possible to build the descriptor separately and and store that struct somewhere else, which I suppose may be potentially a frequent use case in wgpu.

This also gives me an idea that there should be a way to generate a separate "parameters" struct from the function with the bon's attributes syntax.


A cargo-feature gated API definitely makes sense in this case. It allows you to keep the builder API coupled with the existing descriptor structs declarations (which it should be indeed) and this way you avoid the need for publishing a separate crate (and the CI burden it may require).

Conditional compilation is supported just fine with cfg/cfg_attr() in bon.

@ErichDonGubler ErichDonGubler force-pushed the easier-ctors/bon-builder branch 2 times, most recently from 9e4704f to 198be2b Compare October 13, 2024 15:54
@ErichDonGubler ErichDonGubler force-pushed the easier-ctors/bon-builder branch from 198be2b to f7cb6e5 Compare October 23, 2024 16:44
@ErichDonGubler ErichDonGubler force-pushed the easier-ctors/bon-builder branch 3 times, most recently from ff0dbcf to 0df4de8 Compare November 4, 2024 19:38
@Veetaha
Copy link

Veetaha commented Nov 13, 2024

Hi! Small update, I've released a 3.0 version of bon that stabilizes the builder's typestate, makes it much cleaner (it's especially visible in the generated docs) and adds many other new features. This is technically a major release, but breakages are very unlikely (most users will see errors due to rejections of some weird syntax in attributes). I've checked that after bumping the version in this PR everything compiles as before. Here is the release blog post with more details.

@ErichDonGubler ErichDonGubler force-pushed the easier-ctors/bon-builder branch from 0df4de8 to cb068f1 Compare November 14, 2024 03:08
@ErichDonGubler
Copy link
Collaborator Author

@Veetaha: Did the bump, and it still compiles! Any recommendations for new features to take advantage of? 😀

@Veetaha
Copy link

Veetaha commented Nov 14, 2024

Any recommendations for new features to take advantage of?

I think for the use cases here, there aren't obvious game-changer features from the ones added in 3.0.

The only thing I may add is to this comment: #96 (comment).

If you'd like to keep the type alias Label<'a> without inlining it to an Option<&'a str>, then you may use this pattern instead:

#[builder(default, with = |value: &'a str| Some(value))]
pub label: Label<'a>,

The with attribute will override the signature of setters to accept &str and Option<&str>. While with the #[builder(default, into)] in the current diff in this PR the setters would take impl Into<Option<&str>> and Option<impl Into<Option<&str>>.

Other than that there were just some background changes that you got automatically with this update like generated documentation improvements. For example, values specified with #[builder(default)] values are now rendered in docs automatically:
image

/// Debug label of the texture. This will show up in graphics debuggers for easy identification.
#[builder(default, into)]
Copy link

@Veetaha Veetaha Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it's not obvious, but optional fields, that use a generic type are discouraged. This is because it leads to weakened type inference.

In short, if you minimize the problem it boils down to this analogous pattern:

fn example<T>(_value: Option<T>) {}

fn main() {
    example(None);
}

This code doesn't compile with the error:

  |     example(None);
  |     ^^^^^^^ ---- type must be known at this point
  |     |
  |     cannot infer type of the type parameter `T` declared on the function `example`

This similar problem would occur if you use TextureDescriptor::builder() and omit to call the label setter or pass None to maybe_label() setter. The compiler will not be able to infer the generic type L in such case.

I described this problem in detail here, feel free to check that explanation on your leisure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants